Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add corepack project install command #551

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

mmkal
Copy link

@mmkal mmkal commented Aug 21, 2024

Closes #505

This adds a corepack project install command, which is roughly equivalent to

  • corepack enable && npm install for npm projects
  • corepack enable && pnpm install for pnpm projects
  • corepack enable && yarn install for yarn projects

I was able to use it successfully by just running yarn build locally, then I could do node dist/corepack.js project install to verify it works on the corepack repo itself, and node ../corepack/dist/corepack.js project install from various git repos next door.

I wanted to open this up early to get feedback in case the direction isn't considered correct. If it's looking good, I will add tests along similar lines to the existing ones.

Some follow-ons (which I think could be implemented as separate PRs, but could be persuaded to roll them into this one)

  • Common CLI args that all the package managers support:
    • --frozen-lockfile and --no-frozen-lockfile
    • --lockfile-only
    • --no-lockfile
    • --production
    • --dev
    • --offline
    • --force
  • Other subcommands:
    • corepack project add left-pad
    • corepack project remove left-pad
  • Lockfile inference - install with the right package manager based on an existing lockfile (could/should look for package-lock.json vs yarn.lock vs pnpm-lock.yaml etc.). This could be useful in other commands too.

CC @styfle and @aduh95 since you commented on the issue.

Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just needs tests 👍

@mmkal
Copy link
Author

mmkal commented Aug 21, 2024

Looks good to me, just needs tests 👍

Added! Let me know if you think there's more I should check for - there's not much actual new-new code here so hopefully we're good.

Also LMK if you have thoughts on the follow-ups I listed or if you think there should be others. I could get started on some of them.

sources/commands/Project.ts Outdated Show resolved Hide resolved
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Co-authored-by: Steven <[email protected]>
@aduh95
Copy link
Contributor

aduh95 commented Aug 25, 2024

Can you add some documentation please?

@mmkal
Copy link
Author

mmkal commented Aug 30, 2024

@aduh95 added some in 1510e37. Let me know if you think more is needed. As mentioned in the PR body, I am happy to do some followups too. I just want to keep it uncontroversial for now to get this base-level functionality in.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with more documentation

Copy link

@SuperchupuDev SuperchupuDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sources/commands/Project.ts Outdated Show resolved Hide resolved
specifically:
- assert that `stdout` doesn't contain the word "Error"
- use the same npm/pnpm/yarn versions that are already used elsewhere, maybe some mocking is happening
- add a license field
- check for existence of node_modules/ms/package.json
@mmkal
Copy link
Author

mmkal commented Sep 13, 2024

Fixed the tests - they had been working at some point but seemed to have started failing downloading the versions of npm/pnpm/yarn I'd specified, so I just changed them to match versions that are used elsewhere in tests.

Also fixed the NPM->npm casing issue.

@anonrig if you could give another CI approval (I think) this is good to go!

@mmkal mmkal requested a review from anonrig September 22, 2024 14:42
@mmkal
Copy link
Author

mmkal commented Oct 1, 2024

@styfle @anonrig do you know who I should ping to get the workflow/updates re-approved? It is passing for me locally but I would love to avoid this getting conflicted in case there are updates needed.

@styfle
Copy link
Member

styfle commented Oct 1, 2024

@aduh95

@aduh95
Copy link
Contributor

aduh95 commented Oct 1, 2024

It looks like there are some linting errors to address.

tests/Project.test.ts Outdated Show resolved Hide resolved
tests/Project.test.ts Outdated Show resolved Hide resolved
tests/Project.test.ts Outdated Show resolved Hide resolved
@mmkal
Copy link
Author

mmkal commented Oct 7, 2024

@aduh95 @xtian @anonrig sorry about that, looks like my IDE stopped autoformatting somehow. Now fixed, should be good to go (for real this time!).

@mmkal
Copy link
Author

mmkal commented Dec 4, 2024

@aduh95 @xtian @anonrig 👋 , just checking in on this, looks like the code change is approved but the workflow run hasn't been. Could someone click the "approve and run" button so I can make any necessary changes or merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

corepack project install command?
8 participants